Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Velocity computed using negative time offset if no valid position at positive offset. #5349

Merged
merged 5 commits into from
May 24, 2017
Merged

Velocity computed using negative time offset if no valid position at positive offset. #5349

merged 5 commits into from
May 24, 2017

Conversation

klingerj
Copy link

Fixes #4949

@pjcozzi
Copy link
Contributor

pjcozzi commented May 22, 2017

Welcome @klingerj!!!

@emackey
Copy link
Contributor

emackey commented May 22, 2017

To test this, load simple.czml and track one of the satellites with the camera. Grab the blue timeline scrubber and pull it all the way to the right, to the end of the timeline. In master, the camera view goes crazy at the end of the timeline (within one millisecond of the end). In this branch it should be fixed.

Test coverage of EntityView is already poor and unfortunately gets a tiny bit worse with this branch. The few tests there do pass though. Perhaps there should be an issue to improve test coverage of this file overall.

@emackey
Copy link
Contributor

emackey commented May 23, 2017

As predicted, your first merge conflict... 😄

@emackey emackey requested a review from mramato May 23, 2017 15:27
@mramato
Copy link
Contributor

mramato commented May 23, 2017

I'm still seeing incorrect camera behavior in this branch.

  1. Zoom to ISS in simple.czml
  2. Set the time to the slowest multiplier (0.001)
  3. Animate backwards.

The camera will still radically change position.

@emackey
Copy link
Contributor

emackey commented May 23, 2017

Thanks for catching this @mramato!

@klingerj looks like we only tested this with the Molniya satellite, not the ISS. The Molniya uses a high-Earth "North-Up" view (these lines), but the ISS and GeoEye use the VVLH view (these lines).

In the latter case, the yBasis vector comes out negative from what it should be.

We should also test with trucks (Vehicle.czml) and facilities, although I don't expect any problems there due to relative lack of velocity.

@@ -73,7 +82,11 @@ define([
var inertialCartesian = Matrix3.multiplyByVector(toInertial, cartesian, updateTransformCartesian3Scratch5);
var inertialDeltaCartesian = Matrix3.multiplyByVector(toInertialDelta, deltaCartesian, updateTransformCartesian3Scratch6);

Cartesian3.subtract(inertialCartesian, inertialDeltaCartesian, updateTransformCartesian3Scratch4);
if (invertVelocity) {
Cartesian3.subtract(inertialDeltaCartesian, inertialCartesian, updateTransformCartesian3Scratch4);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this more closely, the result here gets fed straight into Cartesian3.magnitude and not used again. So I don't think the invertVelocity flag needs to be checked here at all, as magnitude yields an absolute value. The flag does need to be checked in the VVLH section below for yBasis.

Copy link
Contributor

@emackey emackey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. I think this is fully working now.

@mramato
Copy link
Contributor

mramato commented May 24, 2017

Thanks @klingerj! There was a CHANGES conflict but I took care of it.

@mramato mramato merged commit b6e2d0d into CesiumGS:master May 24, 2017
@klingerj klingerj deleted the stop-time-offset-fixed branch May 24, 2017 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants